Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(config): write temporary vite config to node_modules #18509

Merged
merged 4 commits into from
Nov 1, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 29, 2024

Description

Closes #13267
Closes #9470

After reviewing #17894, I was figuring if there's another way to fix this. Many folks in the linked issues about suggested writing the temporary file to node_modules/.vite, which is what this PR eventually does.

At the time, this solution wasn't feasible because it breaks relative references, node_module resolution, and references between workspace packages. Over time we kinda indirectly addressed them by bundling all relative imports and workspace packages, rewrite all external imports to node_modules, and shim import.meta.*. So in most cases everything is self-contained, except for the rare case of eval() but that's not as common.

So it seems like we can actually put the temporary file in node_modules now, specifically like node_modules/.vite/configs/vite.config.mjs-timestamp-*-*.mjs. I couldn't think of a case where this would break, but I haven't really tested this more robustly either yet.


FWIW I listed some other alternatives and I did make a branch to support solution 2, but this PR may be the better ootb solution if it works.

@sapphi-red
Copy link
Member

This PR does not work with escaped dynamic imports which is one of the reasons we reverted #13269. For example if you prepend the following content in the vite.config.js in playground/define, the config fails to load.

const dynImport = (id) => {
  return import(id)
}
globalThis.__STRINGIFIED_OBJ__ = 'foo'
const result = await dynImport('./commonjs-dep/index.js')
console.log('result', result)

@sapphi-red
Copy link
Member

To fix this, we need to replace dynamic imports with something else and resolve it on our side like #17894 did. I wonder if we can avoid the global function by exposing an internal function from dist/index.js as we have the file in this PR's case.

@bluwy
Copy link
Member Author

bluwy commented Oct 30, 2024

We currently already not have good support for escaped dynamic imports. We bundle all code from different files to a single file, so that means the source files that're within the same directory as vite.config.js will only work right now. This PR does break that they will no longer work, but I think it's rare for someone to do this.

Usually it's also more common for escaped dynamic imports on dependencies rather than paths, and it'll still work in this PR.

Checking the repro in #13730 which caused the revert, it seems to still work well with this PR.

@sapphi-red
Copy link
Member

Ah, that's true. Sounds good to me to try it out.

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

sapphi-red
sapphi-red previously approved these changes Oct 30, 2024
Copy link

pkg-pr-new bot commented Oct 30, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/vite@18509

commit: a591a79

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Christoph tested the PR with Athena Crisis too, and everything looks green. Let's try this out.

@adrian-gierakowski
Copy link

I couldn't think of a case where this would break, but I haven't really tested this more robustly either yet.

this doesn't really help in cases where node_modules are not writable

@adube
Copy link

adube commented Nov 29, 2024

I couldn't think of a case where this would break, but I haven't really tested this more robustly either yet.

this doesn't really help in cases where node_modules are not writable

I am using NIX to manage NodeJS packages, which results in node_modules not being writable.

Upgrading to Vite v6+ results in Vite no longer being able to start because of that.

See also: #18839

@silverwind
Copy link

silverwind commented Nov 30, 2024

node_modules not being writable.

Readonly node_modules will cause issues with not just vite. Many tools write temp files to node_modules/.cache, it's a community standard.

I just wish that vite would also use .cache instead of this .vite-temp. If vite were to use the linked module, one would be able to override with env var CACHE_DIR, which would be a workaround.

@brenc
Copy link

brenc commented Dec 19, 2024

Upgrading to 6.0.3 broke my build today as I also use a readonly node_modules directory. Wouldn't /tmp and the Windows equivalent be the most appropriate places for a temp directory? Or something configurable at least? Out of 1000s of packages Vite is the only one I can think of that tries to write to node_modules and other unexpected locations. I would strongly prefer that packages not do this.

@stefnotch
Copy link
Contributor

Update from the future: #18637 introduces a new option to run Vite without writing a bundled config file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants